Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: SAML: IDP initiated logout request comes with RelayState and it causes error #818

Merged
merged 1 commit into from
Sep 12, 2023
Merged

fix: SAML: IDP initiated logout request comes with RelayState and it causes error #818

merged 1 commit into from
Sep 12, 2023

Conversation

zevitagem
Copy link
Contributor

Proposed changes

Application has only one IDP, if someway we can set default IDP which can be added as RelayState in case its not provided. IDP has flexibility to pass RelayState IDP information during SSO, but not in case of logout request.

Reference: #632

Error

            |   File "/openedx/venv/lib/python3.8/site-packages/social_django/utils.py", line 49, in wrapper
            |     return func(request, backend, *args, **kwargs)
            |   File "/openedx/venv/lib/python3.8/site-packages/social_django/views.py", line 31, in complete
            |     return do_complete(request.backend, _do_login, user=request.user,
            |   File "/openedx/venv/lib/python3.8/site-packages/social_core/actions.py", line 45, in do_complete
            |     user = backend.complete(user=user, *args, **kwargs)
            |   File "/openedx/venv/lib/python3.8/site-packages/social_core/backends/base.py", line 40, in complete
            |     return self.auth_complete(*args, **kwargs)
            |   File "/openedx/venv/lib/python3.8/site-packages/social_core/backends/saml.py", line 313, in auth_complete
            |     idp_name = self.strategy.request_data()['RelayState']
            |   File "/openedx/venv/lib/python3.8/site-packages/django/utils/datastructures.py", line 80, in __getitem__
            |     raise MultiValueDictKeyError(key)
            | django.utils.datastructures.MultiValueDictKeyError: 'RelayState'

Types of changes

Please check the type of change your PR introduces:

  • Release (new release request)
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (PEP8, lint, formatting, renaming, etc)
  • Refactoring (no functional changes, no api changes)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build related changes (build process, tests runner, etc)
  • Other (please describe):

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

@codecov
Copy link

codecov bot commented Aug 1, 2023 •
edited
Loading

Codecov Report

Merging #818 (5808d6d) into master (cf95380) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #818      +/-   ##
==========================================
- Coverage   77.80%   77.79%   -0.02%     
==========================================
  Files         330      330              
  Lines       10097    10100       +3     
  Branches     1195     1195              
==========================================
+ Hits         7856     7857       +1     
- Misses       2087     2089       +2     
  Partials      154      154
Flag Coverage Δ
unittests 77.79% <50.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
social_core/backends/saml.py 80.46% <50.00%> (-1.14%) ⬇️

@nijel
Copy link
Member

nijel commented Aug 2, 2023

#632 talks about providing a fallback value while this PR just raises a different exception. Can you please clarify this? (My SAML knowledge is very limited, so I might be missing something)

@nijel nijel linked an issue Sep 12, 2023 that may be closed by this pull request
SAML: IDP initiated logout request comes with RelayState and it causes error #632
Closed
@nijel nijel mentioned this pull request Sep 12, 2023
Closed
2 tasks
@nijel nijel merged commit 1f60571 into python-social-auth:master Sep 12, 2023
7 of 9 checks passed
@nijel
Copy link
Member

nijel commented Sep 12, 2023

Merged, thanks for your contribution!

@nijel nijel self-assigned this Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nijel nijel

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SAML: IDP initiated logout request comes with RelayState and it causes error
2 participants
@zevitagem @nijel